child_process: create proper public API for channel
#30165
Conversation
addaleax
added
child_process
nodejs-github-bot
added
the
process
|
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2081/ @nodejs/child_process |
|
@vsemozhetbyt If it makes things easier for you, you can feel free to push to my branches directly – your suggestions are always helpful and I trust you to know how the docs should ideally look like :) |
|
@addaleax Thank you) Personally, I am not so sure about my nitpicking myself, and recently I usually review in occasions not so convenient for git operations. But I am grateful for your lingering encouragement) |
| // in progress. Once the user has explicitly requested a certain state, these | ||
| // methods become no-ops in order to not interfere with the user's intentions. | ||
| refCounted() { | ||
| if (++this.#refs === 1 && !this.#refExplicitlySet) { |
There was a problem hiding this comment.
Is refExplicitlySet necessary? Are these *Counted methods? It's the user's responsibility to pair ref and unref calls, balancing internal ref/unref calls is our responsibility. I'm not sure I see why they're needed.
There was a problem hiding this comment.
@bnoordhuis Yeah, I’m not a fan of this either, but without this tests are failing and I wouldn’t know a better solution … the issue isn’t so much that the calls might end up unbalanced, it’s that currently, a process.channel.unref() call for example from userland is expected to unref the channel even if there are event listeners that would otherwise keep it ref()’ed.
|
Oh, and for a bit of meta discussion: I added the control object as a fix for a bug, I didn't otherwise put a lot of thought into it. Since you're thinking of promoting it to public API: do you think it's the best possible interface? |
I think it’s fine … or, rather, I think having an API of the shape |
addaleax
force-pushed
the
child-process-channel-public
branch
from
a1a949b to
8e5f915
Compare
|
edit: although it could also be #25988. |
addaleax
force-pushed
the
child-process-channel-public
branch
from
f3b2b3b to
b47e13e
Compare
|
@lundibundi Addressed your comments and rebased, PTAL |
|
This needs a rebase. |
|
Ping @addaleax |
Instead of exposing the C++ bindings object as `subprocess.channel` or `process.channel`, provide the “control” object that was previously used internally as the public-facing variant of it. This should be better than returning the raw pipe object, and matches the original intention (when the `channel` property was first added) of providing a proper way to `.ref()` or `.unref()` the channel. Refs: nodejs#9322 Refs: nodejs#9313
addaleax
force-pushed
the
child-process-channel-public
branch
from
b47e13e to
ef418e1
Compare
addaleax
added
author ready
This comment has been minimized.
This comment has been minimized.
Instead of exposing the C++ bindings object as `subprocess.channel` or `process.channel`, provide the “control” object that was previously used internally as the public-facing variant of it. This should be better than returning the raw pipe object, and matches the original intention (when the `channel` property was first added) of providing a proper way to `.ref()` or `.unref()` the channel. PR-URL: #30165 Refs: #9322 Refs: #9313 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in e65bed1 🎉 |
Instead of exposing the C++ bindings object as
subprocess.channelor
process.channel, provide the “control” object that waspreviously used internally as the public-facing variant of it.
This should be better than returning the raw pipe object, and
matches the original intention (when the
channelproperty wasfirst added) of providing a proper way to
.ref()or.unref()the channel.
Refs: #9322
Refs: #9313
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes